-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MUSICA Tutorial Chapter 1 #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
CMakeLists.txt
Outdated
@@ -70,6 +70,7 @@ elseif(${CMAKE_Fortran_COMPILER_ID} MATCHES "GNU") | |||
elseif(${CMAKE_Fortran_COMPILER_ID} MATCHES "PGI") | |||
list(APPEND musica_compile_definitions MUSICA_USING_PGI) | |||
endif() | |||
set(CMAKE_Fortran_FLAGS "-fPIE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we need to set this, then please set it with [target_compile_options](https://cmake.org/cmake/help/latest/command/target_compile_options.html)
on the appropriate target (musica fortran?). This prevents us from polluting downstream builds with options that they may not want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken this out for now ... I don't understand why I needed it on one Linux OS (RedHat) with GCC 13.2 but not on another (Ubuntu) with GCC 11.4.
Co-authored-by: Kyle Shores <kyle.shores44@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just one question and one suggestion.
docs/source/tutorial/chapter1.rst
Outdated
program demo | ||
use musica_util, only: string_t | ||
use musica_micm, only: get_micm_version | ||
implicit none | ||
type(string_t) :: micm_version | ||
micm_version = get_micm_version() | ||
print *, "MICM version ", micm_version%get_char_array() | ||
end program demo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest that we move all the tutorial code snippets to separate source code files that get compiled and run as tests. This way we can ensure that if we break a part of the tutorial with updates to the MUSICA source code, a test will fail.
We do something similar in MICM:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let me do that ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the overview!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better! Just one last comment
program test_get_micm_version | ||
use musica_util, only: string_t | ||
use musica_micm, only: get_micm_version | ||
implicit none | ||
type(string_t) :: micm_version | ||
micm_version = get_micm_version() | ||
print *, "MICM version ", micm_version%get_char_array() | ||
end program test_get_micm_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I would just now pull the content of this test directly into the tutorial so they don't get out-of-sync. Similar to this:
https://github.com/NCAR/micm/blob/ee5c03e904d9dc3d60619dc9dbce06232bd32852/docs/source/user_guide/rate_constant_tutorial.rst?plain=1#L46-L49
Also, I would move this to a test/tutorial/
folder or something similar
Now using literal include for test_get_micm_version.F90 in Chapter 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
Tutorial Chapter 1, linking a fortran program to the MUSICA library.,
Added get_micm_version function to the fortran interface.